Add whatprovides resolution to verify-install plugin#4804
Add whatprovides resolution to verify-install plugin#4804vaibhavdaren wants to merge 2 commits intomainfrom
whatprovides resolution to verify-install plugin#4804Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the resolution of virtual RPM provides to actual package names using 'rpm -q --whatprovides' during artifact preparation and installation verification. In 'verify_installation.py', replace the dictionary comprehension for 'verify_map' with a loop using 'setdefault' and 'tmt.utils.uniq' to prevent overwriting entries when multiple virtual provides resolve to the same package name.
725fbcf to
1d6cf0d
Compare
9914613 to
d189b00
Compare
Allow resolving packages from file paths (e.g. /usr/bin/make) by leveraging DNF provides/whatprovides instead of requiring explicit package names.
d1de079 to
e08283d
Compare
whatprovides resolution to verify-install plugin
|
The test failures are unrelated to the change. Flaky Test : /tests/execute/restart/with-reboot 🔎 /packit retest-failed |
| The script must emit one line per capability in the same order as the input. | ||
| Each line is either a valid NEVRA string for a found capability, or an error | ||
| message (e.g. ``no package provides <cap>``) for one that is not provided by | ||
| any installed package. |
There was a problem hiding this comment.
The capabilities name is neither generic enough, nor is it correct in the dnf context. For dnf context, provides could be a better fit for what this is.
There was a problem hiding this comment.
changed to provides.
context: for choosing capability. We already have a provider.
| :param capabilities: Capabilities to resolve — package names, file paths, or | ||
| virtual provides (e.g. ``make``, ``/usr/bin/make``, ``pkgconfig(openssl)``). |
There was a problem hiding this comment.
Why the em-dash. Personally, if one uses em-dash, it feels like they did not consider the wording at all.
|
|
||
| :param capabilities: Capabilities to resolve — package names, file paths, or | ||
| virtual provides (e.g. ``make``, ``/usr/bin/make``, ``pkgconfig(openssl)``). | ||
| :returns: A shell script whose stdout contains one NEVRA line per capability. |
There was a problem hiding this comment.
Again, too technical. The whose stdout is not relevant and too literal. See the other docstrings
| :param capabilities: Capabilities to resolve — package names, file paths, or | ||
| virtual provides (e.g. ``make``, ``/usr/bin/make``, ``pkgconfig(openssl)``). | ||
| :returns: A shell script whose stdout contains one NEVRA line per capability. | ||
| :raises NotImplementedError: If the package manager does not support this query. |
There was a problem hiding this comment.
See the raises just below here. Be consistent.
| result[package] = parts[1] if len(parts) == 2 else SpecialPackageOrigin.UNKNOWN | ||
| return result | ||
|
|
||
| def resolve_capabilities(self, capabilities: Iterable[str]) -> 'dict[str, Optional[Version]]': |
There was a problem hiding this comment.
Incorrect quotations of the return type
|
|
||
| def resolve_capabilities(self, capabilities: Iterable[str]) -> 'dict[str, Optional[Version]]': | ||
| """ | ||
| Map each capability to the :py:class:`Version` of the installed package providing it. |
There was a problem hiding this comment.
| Map each capability to the :py:class:`Version` of the installed package providing it. | |
| Map each capability to the :py:class:`Version` of the packages providing it. |
- The package is not installed when you need to call this
- Multiple packages can provide the same provides, particularly if the user did not add relevant constraints
| try: | ||
| output = self.guest.execute(script) | ||
| stdout = output.stdout | ||
| except tmt.utils.RunError as exc: | ||
| stdout = exc.stdout |
There was a problem hiding this comment.
This is just wrong isn't it? Can't figure out what you are trying to catch here.
| except tmt.utils.RunError as exc: | ||
| stdout = exc.stdout | ||
|
|
||
| result: dict[str, Optional[Version]] = dict.fromkeys(caps, None) |
There was a problem hiding this comment.
Move this at the top and you can avoid the intermediate caps. I commented about this very recently didn't I?
| with contextlib.suppress(ValueError): | ||
| result[cap] = RpmVersion.from_nevra(line.strip()) |
There was a problem hiding this comment.
No contextlib.suppress. There is a special case being handled, then document it properly
There was a problem hiding this comment.
Sorry - just seeing this now, but prolly made a duplicate comment: #4804 (comment)
| def resolve_capabilities(self, *capabilities: str) -> ShellScript: | ||
| # Reuse the existing rpm --whatprovides script; output is one NEVRA per line, | ||
| return self._construct_presence_script(*[Package(c) for c in capabilities]) |
There was a problem hiding this comment.
Not a good approach, see the other comments. The repoquery might give you more flexibility on the format and allow you to get more information to process further like the repoid.
There was a problem hiding this comment.
We now , run repoquery for each of the packages in a script. Which is actually easier to parse. Addressed in 0fb9b50.
|
|
||
| result: dict[str, Optional[Version]] = dict.fromkeys(caps, None) | ||
| for cap, line in zip(caps, (stdout or '').splitlines()): | ||
| with contextlib.suppress(ValueError): |
There was a problem hiding this comment.
This supresses parsing errors (and others too) - ambigous. Suggest adding a debug() log here:
for cap, line in zip(caps, (stdout or '').splitlines()):
try:
result[cap] = RpmVersion.from_nevra(line.strip())
except ValueError:
self.debug(f"Could not resolve capability '{cap}': {line.strip()}")
Use whatprovides lookup for file-based dependencies
Allow resolving packages from file paths (e.g. /usr/bin/make)
by leveraging DNF provides/whatprovides instead of requiring
explicit package names.